Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add consistency to output handling in runtime code execution #583

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add consistency to output handling in runtime code execution #583

wants to merge 1 commit into from

Conversation

rjanja
Copy link

@rjanja rjanja commented Nov 1, 2018 •
edited
Loading

Summary of changes

The node execution function eval/2 in Mix.Releases.Runtime.Control has been modified to be consistent with rpc/2 with regard to how it handles the result.

In rpc, the result is always printed to stdout. But in eval nothing is printed.

Current behavior

Explicit IO

$ ./bin/myapp rpc --mfa "IO.inspect/1" "foo"
"foo"
"foo"
$ ./bin/myapp eval --mfa "IO.inspect/1" "foo"
"foo"

Without explicit IO

$ ./bin/myapp rpc --mfa "Enum.join/1" --argv -- foo bar
"foobar"
$ ./bin/myapp eval --mfa "Enum.join/1" --argv -- foo bar
$

I expected "foobar" to be printed.

I suppose the inconsistency could instead be fixed by never printing the result, but that would make these commands less useful I think. If silent operation is desired, perhaps that could be added later as an optional flag.

Proposed behavior

Explicit IO

$ ./bin/myapp rpc --mfa "IO.inspect/1" "foo"
"foo"
"foo"
$ ./bin/myapp eval --mfa "IO.inspect/1" "foo"
"foo"
"foo"

Without explicit IO

$ ./bin/myapp rpc --mfa "Enum.join/1" --argv -- foo bar
"foobar"
$ ./bin/myapp eval --mfa "Enum.join/1" --argv -- foo bar
"foobar"

Checklist

  • New functions have typespecs, changed functions were updated
  • Same for documentation, including moduledocs
  • Tests were added or updated to cover changes
  • Commits were squashed into a single coherent commit

Also:

  • Thank you bitwalker for all your hard work on Distillery!

Licensing/Copyright

I certify that I own, and have sufficient rights to contribute, all source code and
related material intended to be compiled or integrated with the source code for Distillery
(the "Contribution"). My Contribution is licensed under the MIT License.

@rjanja rjanja force-pushed the eval-output branch 2 times, most recently from 10e12b6 to 7a6ae1e Compare November 1, 2018 05:19
@rjanja
Copy link
Author

rjanja commented Nov 1, 2018

I've been able to get the tests running locally, and the four failures reported by AppVeyor are passing for me. Not sure about those other skipped tests ("cannot run clustered test cases if distribution is not active"). I've tried kicking the build off a few times and it's the same failures every time.

@bitwalker
Copy link
Owner

I think is probably reasonable - though I think my preference would probably be to always require explicit I/O for eval, but implicit for rpc. The reason is that they are generally used for different things. The reason eval requires explicit I/O is because you generally use it to build CLIs, or at least scripts to back custom comands; and in such cases you want control over the output. On the other hand, rpc is generally used to interrogate the remote node, so it is more convenient to have the result always be printed. If we really want to unify them, I would prefer to have rpc also do what eval did before, which is require explicit I/O to print the result. That said, I would like to know your thoughts on what I've just described, and whether your use cases differ, and if so, how; that may help me form a better mental model for what this should look like :)

@rjanja
Copy link
Author

rjanja commented Nov 7, 2018

The distinction makes a little more sense now that I understand the reasoning behind it. I think I'd still favor consistent I/O handling, or a command line flag to enable (or disable, if enabled was the default) the printing of return value.

One point of confusion (for me) was that eval and rpc essentially do the same thing, just in different environments (local vs remote node). The names of these tasks being so different made it harder to grok.

As for my use case, I'm adding a couple of helpers to Bootleg for invoking these release tasks with arguments. It's a WIP but the syntax is currently:

  • mix bootleg.eval mfa HelloWorld.ReleaseTasks.migrate/0
  • mix bootleg.rpc mfa Application.loaded_applications/0
  • mix bootleg.rpc expr ':application.get_key(:hello_world, :vsn)'

Would it be overkill to have as Distillery tasks:

  • run_local
  • run_remote
  • eval_local
  • eval_remote

Where eval_* doesn't print but run_* does?

I'm happy to move this discussion to a new issue, and pare this PR back to just some minor documentation improvements. Let me know what you think. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rjanja @bitwalker